Fix: TAS assignment error #3937

kerthcet · 2025-01-07T05:08:03Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix building TAS assignments for workloads with multiple PodSets (eg. JobSet or kubeflow Jobs). The assignment was computed independently for the PodSets which could result in conflicts rendering the pods unschedulable by the kube-scheduler.

Signed-off-by: kerthcet <[email protected]>

k8s-ci-robot · 2025-01-07T05:08:06Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

kerthcet · 2025-01-07T05:08:10Z

/test

k8s-ci-robot · 2025-01-07T05:08:13Z

@kerthcet: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test pull-kueue-build-image-main

/test pull-kueue-test-e2e-main-1-29

/test pull-kueue-test-e2e-main-1-30

/test pull-kueue-test-e2e-main-1-31

/test pull-kueue-test-integration-main

/test pull-kueue-test-multikueue-e2e-main

/test pull-kueue-test-scheduling-perf-main

/test pull-kueue-test-tas-e2e-main

/test pull-kueue-test-unit-main

/test pull-kueue-verify-main

Use /test all to run all jobs.

In response to this:

/test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

kerthcet · 2025-01-07T05:08:20Z

/test all

netlify · 2025-01-07T05:08:24Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`3b650c2`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/678602a4cb1f5600087f8453

Signed-off-by: kerthcet <[email protected]>

kerthcet · 2025-01-07T05:15:29Z

/test all

mimowo

Please add a unit test in scheduler_test.go TestScheduleForTAS for the problematic scenario.

mimowo · 2025-01-07T08:06:13Z

pkg/cache/tas_flavor_snapshot.go

+		for k, v := range singlePodRequest {
+			usage[k] = v * int64(domain.state)
+		}
+		s.addUsage(domain.id, usage)


I believe this will work fine for now as TAS is not combined with cohorts, and so only one workload is considered in each scheduling cycle on the snapshot. However, this will not work well when we start supporting cohorts - in that case usage coming from the assignment phase for a workload that is de-prioritized in a cohort will consume the capacity, but scheduler should operate on "clean" capacity.

One way to go about it in the future would be to remember usage coming from "Inflight" PodSets, and clean it at the end of the assignment phase, before the actual scheduling phase.

Since I don't see it problematic in the current assumptions (no cohorts) I believe a TODO with #3761 is sufficient.

Will add tests later.

Do you mean when having time, or in a follow up PR? My preference would be to add the test in this PR.

I mean when I have time later, have codes without tests is terrible.

Sure, I would like to include the fix in 0.10.1, and tentatively planning next week. Do you think you would add the test by then? I can also help to write it

will do this weekend

Can we have a test that covers a case where request topology is not the lowest level? E.g. the lowest level is hostname and job request (and can only fit in) a block? I have concerns if it's gonna work if the requested level is not the lowest one, since s.addUsage() method add usage only to leaves in the topology tree structure

Make sense, but I think it still works because we'll traverse down the domains level by level until the lowest ones, which means we'll always get the lowest domains in addUsage(). Added the tests. PTAL.

Can you adjust the test so the topology is made out of three levels. And then add also another podset that should make a workload not fit within requested topology? I am afraid this usage may not be counted if it's not at the lowest level, so this is what I wanted to test

After second consideration I think, you're actually right and the usage will be tracked correctly. Thanks for adding the test

kerthcet · 2025-01-13T08:08:42Z

Test added, PTAL.
Before it will fail as blow, which proves the bug is fixed.

              				TopologyAssignment: &v1beta1.TopologyAssignment{
              					Levels: {"kubernetes.io/hostname"},
              					Domains: []v1beta1.TopologyDomainAssignment{
              						{
              							Values: {"x1"},
            - 							Count:  7,
            + 							Count:  8,
              						},
              						{
              							Values: {"y1"},
            - 							Count:  8,
            + 							Count:  7,
              						},
              					},
              				},

Signed-off-by: kerthcet <[email protected]>

pkg/cache/tas_flavor_snapshot.go

Signed-off-by: kerthcet <[email protected]>

PBundyra · 2025-01-14T15:31:27Z

/lgtm
cc @mimowo

k8s-ci-robot · 2025-01-14T15:31:35Z

LGTM label has been added.

Git tree hash: 443aefcbe7ebde6a093ee0181c3a61236f54ce8c

mimowo · 2025-01-14T15:33:31Z

/approve

k8s-ci-robot · 2025-01-14T15:33:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kerthcet, mimowo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mimowo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mimowo · 2025-01-14T15:34:04Z

/cherry-pick release-0.10 release-0.9

k8s-infra-cherrypick-robot · 2025-01-14T15:34:07Z

@mimowo: once the present PR merges, I will cherry-pick it on top of release-0.10 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.10 release-0.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-infra-cherrypick-robot · 2025-01-14T15:35:14Z

@mimowo: new pull request created: #3970

In response to this:

/cherry-pick release-0.10 release-0.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

mimowo · 2025-01-14T15:39:42Z

Proposal:
/release-note-edit

Fix building TAS assignments for workloads with multiple PodSets (eg. JobSet or kubeflow Jobs). The assignment was computed independently for the PodSets which could result in conflicts rendering the pods unschedulable by the kube-scheduler.

Fix: TAS assignment error

d33865f

Signed-off-by: kerthcet <[email protected]>

k8s-ci-robot requested review from mbobrovskyi and PBundyra January 7, 2025 05:08

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 7, 2025

fix nil map

4f61106

Signed-off-by: kerthcet <[email protected]>

mimowo reviewed Jan 7, 2025

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 8, 2025

kerthcet force-pushed the fix/tas branch from 0274ecd to 4f61106 Compare January 8, 2025 02:23

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 8, 2025

mimowo mentioned this pull request Jan 9, 2025

Release v0.10.1 #3950

Closed

34 tasks

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 13, 2025

kerthcet marked this pull request as ready for review January 13, 2025 08:09

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 13, 2025

k8s-ci-robot requested a review from mimowo January 13, 2025 08:09

Add test

54731b2

Signed-off-by: kerthcet <[email protected]>

kerthcet force-pushed the fix/tas branch from 21fa434 to 54731b2 Compare January 13, 2025 08:18

PBundyra reviewed Jan 13, 2025

View reviewed changes

pkg/cache/tas_flavor_snapshot.go Outdated Show resolved Hide resolved

Add another test

3b650c2

Signed-off-by: kerthcet <[email protected]>

k8s-ci-robot assigned PBundyra Jan 14, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 14, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 14, 2025

k8s-ci-robot merged commit d638e2a into kubernetes-sigs:main Jan 14, 2025
17 checks passed

k8s-ci-robot added this to the v0.11 milestone Jan 14, 2025

k8s-infra-cherrypick-robot mentioned this pull request Jan 14, 2025

[release-0.10] Fix: TAS assignment error #3970

Merged

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jan 14, 2025

kerthcet mentioned this pull request Jan 15, 2025

[release-0.9] Fix: TAS assignment error #3972

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: TAS assignment error #3937

Fix: TAS assignment error #3937

kerthcet commented Jan 7, 2025 •

edited by k8s-ci-robot

Loading

k8s-ci-robot commented Jan 7, 2025

kerthcet commented Jan 7, 2025

k8s-ci-robot commented Jan 7, 2025

kerthcet commented Jan 7, 2025

netlify bot commented Jan 7, 2025 •

edited

Loading

kerthcet commented Jan 7, 2025

mimowo left a comment

mimowo Jan 7, 2025

kerthcet Jan 7, 2025

mimowo Jan 8, 2025

kerthcet Jan 8, 2025

mimowo Jan 9, 2025

kerthcet Jan 10, 2025

PBundyra Jan 13, 2025

kerthcet Jan 14, 2025

PBundyra Jan 14, 2025 •

edited

Loading

PBundyra Jan 14, 2025

kerthcet commented Jan 13, 2025 •

edited

Loading

PBundyra commented Jan 14, 2025

k8s-ci-robot commented Jan 14, 2025

mimowo commented Jan 14, 2025

k8s-ci-robot commented Jan 14, 2025

mimowo commented Jan 14, 2025

k8s-infra-cherrypick-robot commented Jan 14, 2025

k8s-infra-cherrypick-robot commented Jan 14, 2025

mimowo commented Jan 14, 2025 •

edited

Loading

Fix: TAS assignment error #3937

Fix: TAS assignment error #3937

Conversation

kerthcet commented Jan 7, 2025 • edited by k8s-ci-robot Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Jan 7, 2025

kerthcet commented Jan 7, 2025

k8s-ci-robot commented Jan 7, 2025

kerthcet commented Jan 7, 2025

netlify bot commented Jan 7, 2025 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

kerthcet commented Jan 7, 2025

mimowo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PBundyra Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerthcet commented Jan 13, 2025 • edited Loading

PBundyra commented Jan 14, 2025

k8s-ci-robot commented Jan 14, 2025

mimowo commented Jan 14, 2025

k8s-ci-robot commented Jan 14, 2025

mimowo commented Jan 14, 2025

k8s-infra-cherrypick-robot commented Jan 14, 2025

k8s-infra-cherrypick-robot commented Jan 14, 2025

mimowo commented Jan 14, 2025 • edited Loading

kerthcet commented Jan 7, 2025 •

edited by k8s-ci-robot

Loading

netlify bot commented Jan 7, 2025 •

edited

Loading

PBundyra Jan 14, 2025 •

edited

Loading

kerthcet commented Jan 13, 2025 •

edited

Loading

mimowo commented Jan 14, 2025 •

edited

Loading